-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix IOSHAContentSkinLayer registration #244
Conversation
Otherwise you get an error in the tests: ``` KeyError: 'Profile "profile-osha.oira:default" requires the dependency-profile "profile-plone.restapi:default", which does not exist.' ```
This is mysterious, but I think the biggest issue is that the IOSHAContentSkinLayer overrides the NuPlone and not the Euphorie one. See:
Can you test if my suspect is right? |
How would you test that? What issue do you expect? |
I would revert the change you are proposing and add the modification I am proposing.
The answer to your question is "None". The package The layers should respect that hierarchy in order for the customizations to work properly. |
And then what? Just making that change will not do anything. I will at least have to start up the instance, and probably call some view or whatever.
OK and how does that manifest? That sounds very theoretical. I need some issue that I can reproduce so that I can verify that it is fixed by the change you're proposing. |
Wait a minute, are you still talking about the issue that the IOSHAContentSkinLayer is not active? When you wrote “This is mysterious, but I think the biggest issue is ...” I thought you were referring to a new issue, but maybe I misunderstood that? |
I've changed the inheritance for good measure in e7333cf, but as soon as I restore that interface declaration in the zcml the quaive-edit view returns 404 again. |
I think you figure it out all by yourself by now. |
Question: did you install first osha.oira and then Euphorie? |
I think so. I assumed that euphorie would be installed as a dependency of osha.oira, but IIRC it wasn't and I installed euphorie manually afterwards. Do you think that caused the issue? How? |
This caught me off guard as well. Can you review #246 and #247 please?
Yes, and I can confirm by installing the packages in the proper order.
Because the layers are layered :D If you have Does this explanation seems reasonable to you? |
Ah, I didn't know there was an order of precedence. That would explain some things. But if we make IOSHAContentSkinLayer inherit from IEuphorieContentLayer then the more specific layer (IOSHAContentSkinLayer) will take precedence, regardless of the install order, right? #247 claims to be only about tests, but I saw now that it also changes the profile dependencies and interface inheritance. That's a bit sneaky, but OK, I noticed. I'll double check whether this fixes the issue for me as well, and if so, I'm fine with that! |
I think this is already sorted out in another way and anyway the PR is not in a mergeable state. |
Ref syslabcom/scrum#1979